Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Bookmark manager style update #4658

Merged
merged 6 commits into from
Oct 11, 2016
Merged

Bookmark manager style update #4658

merged 6 commits into from
Oct 11, 2016

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Oct 11, 2016 •
edited
Loading

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fixes #4562
Fixes #1994
Fixes #4523

Completely updated styles

Almost done- needs some tweaks
screen shot 2016-10-11 at 2 02 04 am

Test Plan:

  1. Visit about:bookmarks
  2. Test the functionality:
    • Try drag and dropping (both folders and bookmarks and between each)
    • Try right click > context menu on bookmarks
    • Try clicking the import button
    • Try resizing the window; how does it look?
    • Use the search in the top right; ensure this works as expected

@bsclifton
Updates to about:bookmarks
eea84e5 
- Page is now styled! It looks similar to about:history
- Bookmark entries now shown in sortableTable (can't be re-ordered, but can be sorted)
- favicons are now shown
- URL is shown to right of bookmark on mouse over

Fixes #4562
Fixes #1994
@bsclifton
Integrated with multi-select (had to make a few tweaks)
d7be320
@bsclifton
Optimization for history page. Only one call per history date is made…
b469473 
… now.

Auditors: @darkdh
@bsclifton
Bookmark manager no longer opens multiple tabs when double clicked
bc63ccd 
Fixes #4523

Also includes:
Update color of about:bookmarks from "been gone hunting" green to orange
@bsclifton bsclifton added this to the 0.12.5dev milestone Oct 11, 2016
@bsclifton bsclifton self-assigned this Oct 11, 2016
@bsclifton
The refactoring done in #4361 unfortunately broke autofill.
41a1087 
This commit reverts the parts that broke it and keeps the parts that were OK.

Auditors: @darkdh @bridiver

Test Plan: follow steps from original PR
@bsclifton
Reworking about:bookmarks after feedback from Brad. Still some more t…
3b730b9 
…o do.
@bsclifton
Copy link
Member Author

bsclifton commented Oct 11, 2016 •
edited
Loading

Almost there- one more session it'll be knocked out. All the code is done- just needs some style updates / tweaks.

@bbondy since it requires additional work and we're in a code freeze, we can always kick to 0.12.6. Let me know

bbondy
bbondy reviewed Oct 11, 2016
View reviewed changes
docs/windowActions.md
@@ -891,7 +891,7 @@ See `eventStore.js` for an example use-case



### onMouseOverBookmarkFolder(folderId, bookmarks, bookmark, xPos, yPos)
### setBookmarksToolbarSelectedFolderId(folderId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting I think we're trying to move away from setter actions

@bbondy
Copy link
Member

bbondy commented Oct 11, 2016

++

@bbondy bbondy merged commit c1fffde into brave:master Oct 11, 2016
@bradleyrichter
Copy link
Contributor

Be sure to ref #4562 for a few more changes...

@bsclifton bsclifton mentioned this pull request Oct 11, 2016
Closed
@bsclifton bsclifton deleted the bookmark-manager-style-update branch October 11, 2016 14:56
darkdh
darkdh reviewed Oct 11, 2016
View reviewed changes
js/components/sortableTable.js
if (targetElement.tagName === 'TR') break
targetElement = targetElement.parentNode
}
if (!targetElement) return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice for parent traversing!!!

darkdh
darkdh reviewed Oct 11, 2016
View reviewed changes
js/components/sortableTable.js
@@ -81,7 +87,12 @@ class SortableTable extends ImmutableComponent {
? (typeof this.props.totalRowObjects[parseInt(tableID)][index].toJS === 'function'
? this.props.totalRowObjects[parseInt(tableID)][index].toJS()
: this.props.totalRowObjects[parseInt(tableID)][index])
: null
: (this.props.rowObjects.size > 0 || this.props.rowObjects.length > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of non multi table selection. 👍

@bsclifton bsclifton changed the title WIP: Bookmark manager style update Bookmark manager style update Oct 11, 2016
@bsclifton
Copy link
Member Author

Tests were added here:
#4690

@bsclifton bsclifton mentioned this pull request Oct 11, 2016
Closed
This was referenced Oct 12, 2016
Closed
Closed
Closed
Closed
This was referenced Oct 13, 2016
Closed
Closed
This was referenced Oct 13, 2016
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darkdh darkdh darkdh left review comments

@bbondy bbondy bbondy left review comments

Assignees

@bsclifton bsclifton

Labels
feature/bookmarks QA/checked-macOS QA/checked-Win64 QA/test-plan-specified
Projects
None yet
Milestone
0.12.5dev
Development

Successfully merging this pull request may close these issues.
6 participants
@bsclifton @bbondy @bradleyrichter @darkdh @alexwykoff @srirambv